-
-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some fixes #1941
Some fixes #1941
Conversation
Signed-off-by: Steffen Jaeckel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks fine, the only thing I don't understand is Fix libstrophe verbosity level always being set to 0 on start . What exactly is being fixed here?
Do you remove the line because we already require a later libstrophe version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR, generally nice, though I would like to see more readability and consistency in changes. I tried to read the code without commit message and it was a bit hard, which can make further maintenance a bit harder.
}; | ||
|
||
static void | ||
_win_find(char* key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it appears that the name mismatches the purpose, maybe rather something like _mam_win_match_add
?
cur); | ||
cur = next; | ||
} | ||
struct iq_win_finder st = { 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undescriptive variable name, something like matcher_data
would be good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also another suggestion: make a blank space between }
and struct
, since these are 2 different logical parts (1 part to remove window, other to remove iq handlers, maybe these should even be separated on 2 functions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, it's a bit strange approach to make your own dynamic array and structure just for this, why not something like this for readability? (let's call it pseudocode
since it's not tested)
static void
_mam_win_match_add(char* key, ProfIqHandler* handler, GArray* to_be_removed)
{
if (handler->func == _mam_rsm_id_handler) {
// Append the element to the array
g_array_append_val(to_be_removed, g_strdup(key));
}
}
GArray* to_be_removed = g_array_new(FALSE, FALSE, sizeof(char*));
// Iterate through id_handlers and call _mam_win_match_add for each entry to identify windows to be removed.
g_hash_table_foreach(id_handlers, (GHFunc)_mam_win_match_add, to_be_removed);
// Access elements using g_array_index and free each element
for (guint n = 0; n < to_be_removed->len; ++n) {
char* to_remove = g_array_index(to_be_removed, char*, n);
g_hash_table_remove(id_handlers, to_remove);
}
g_array_free(to_be_removed, TRUE);
@@ -1261,7 +1261,7 @@ _handle_muc_private_message(xmpp_stanza_t* const stanza) | |||
ProfMessage* message = message_init(); | |||
message->type = PROF_MSG_TYPE_MUCPM; | |||
|
|||
const gchar* from = xmpp_stanza_get_from(stanza); | |||
const char* from = xmpp_stanza_get_from(stanza); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few more cases, I suggest to catch all of them, using regex: gchar.+xmpp_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to open another PR with those changes.
@@ -267,8 +268,56 @@ iq_handlers_init(void) | |||
rooms_cache = g_hash_table_new_full(g_str_hash, g_str_equal, free, (GDestroyNotify)xmpp_stanza_release); | |||
} | |||
|
|||
struct iq_win_finder | |||
{ | |||
gsize max, cur; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max
can be more descriptive? E.g. max_size
The reason for that line was the missing init of the verbosity variable in an earlier release of libstrophe. 1. This has been fixed since and 2. we set the verbosity level now as well in |
Can you add this to the commit message? Would make it easier to understand later :) |
Fixup/revert of e55f6d7 This line had been added because an earlier release of libstrophe missed the initialisation of that variable, which has since been fixed. Signed-off-by: Steffen Jaeckel <[email protected]>
Signed-off-by: Steffen Jaeckel <[email protected]>
Before this patch the following scenario lead to a segfault: 1. open a window that sends a MAM request 2. fast enough close that window again before the MAM response was processed Once the MAM response is received we'd call `_mam_rsm_id_handler()` from the `_iq_handler()` and `window` would point to a non-existant window which leads to a segfault. Signed-off-by: Steffen Jaeckel <[email protected]>
Reset the `received_disco_items` flag when initializing the iq module. This has caused the console error message "Server doesn't support MAM" sometimes on reconnect. Fixes #1940 Signed-off-by: Steffen Jaeckel <[email protected]>
1. close logfile as last action 2. Fix `plugins_shutdown()` accessing `((ProfPlugin*)curr->data)->lang` after `curr->data` had already potentially been free'd. Signed-off-by: Steffen Jaeckel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestions.
As they're mere opinions and don't really fix anything, I suggest to merge as is.
@@ -1261,7 +1261,7 @@ _handle_muc_private_message(xmpp_stanza_t* const stanza) | |||
ProfMessage* message = message_init(); | |||
message->type = PROF_MSG_TYPE_MUCPM; | |||
|
|||
const gchar* from = xmpp_stanza_get_from(stanza); | |||
const char* from = xmpp_stanza_get_from(stanza); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to open another PR with those changes.
Check commit messages for details.